-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
REF: use BlockManager.apply for Rolling.count #35883
REF: use BlockManager.apply for Rolling.count #35883
Conversation
results = [] | ||
for b in blocks: | ||
result = b.notna().astype(int) | ||
_, obj = self._create_blocks(self._selected_obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually hoping that count could be defined in terms of self._apply
so we can have less custom logic
def count(self):
window_func = self._get_cython_func_type("roll_count")
return self._apply(window_func, center=self.center, floor=0, name="count", **kwargs)
Might be not as straightforward (possibly) but just a heads up of where (hopefully) count should head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this something you're planning on doing in the forseeable future? i agree thatd be a nicer solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll experiment with this tonight and get a PR up if tests pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this version allows (inconsistently with other rolling function) non-numeric data like datetimes and strings because of the ahead of time coercing with notna().astype(int)
. Taking longer than expected to tie out tests with using the roll_count
cython version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if anything can be shared, but it looks like DataFrameGroupBy.count
is doing things kludgily blockwise and should also use an apply pattern
…f-count-blockwise
Thanks @jbrockmendel. Couldn't find a straightforward simplification immediately, so this is a nice cleanup. |
* REF: remove unnecesary try/except * TST: add test for agg on ordered categorical cols (pandas-dev#35630) * TST: resample does not yield empty groups (pandas-dev#10603) (pandas-dev#35799) * revert accidental rebase * REF: use BlockManager.apply for Rolling.count Co-authored-by: Karthik Mathur <[email protected]> Co-authored-by: tkmz-n <[email protected]>
* REF: remove unnecesary try/except * TST: add test for agg on ordered categorical cols (pandas-dev#35630) * TST: resample does not yield empty groups (pandas-dev#10603) (pandas-dev#35799) * revert accidental rebase * REF: use BlockManager.apply for Rolling.count Co-authored-by: Karthik Mathur <[email protected]> Co-authored-by: tkmz-n <[email protected]>
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff